Skip to content

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Sep 26, 2025

The default rotation in the 3d viewport changed for some reason. Also saving the annotation became necessary because changing the zoom leads to an unsaved state which left the CI hanging due to a confirm dialog. I don't know why this wasn't a problem earlier.

Steps to test:

@philippotto philippotto self-assigned this Sep 26, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 26, 2025

📝 Walkthrough

Walkthrough

Converts several in-page evaluate callbacks to async, awaits API readiness, dispatches actions, and calls api.tracing.save() (via returned api or window.webknossos.DEV.api.tracing.save()) after dispatches so tracing is persisted before the evaluated functions resolve.

Changes

Cohort / File(s) Summary
Puppeteer test async tracing
frontend/javascripts/test/puppeteer/dtype_datasets_rendering.screenshot.ts
Convert page.evaluate callbacks to async; await API readiness; dispatch actions; call and await api.tracing.save() (either from returned api or window.webknossos.DEV.api) after dispatches in multiple control paths; minor control-flow and punctuation tweaks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

testing, CI, frontend

Suggested reviewers

  • hotzenklotz

Poem

I nibble keys and make it neat,
Await the API, then dispatch, complete.
A trace is saved, a small delight,
Tests hum softly through the night. 🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title “Fix nightly by updating screenshots” succinctly captures the core purpose of the changeset—updating screenshot tests to resolve failures in the nightly build—and directly reflects the main adjustments made to accommodate viewport rotation and annotation saving in CI.
Description Check ✅ Passed The description explains the rationale behind the changes—namely a shift in the default 3D viewport rotation and the need to save annotations to avoid CI hangs—which corresponds directly to the modifications in the screenshot tests and matches the changes summarized in the pull request.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-nightly-25-09-26

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5e403d4 and 1db46b9.

📒 Files selected for processing (1)
  • frontend/javascripts/test/puppeteer/dtype_datasets_rendering.screenshot.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/javascripts/test/puppeteer/dtype_datasets_rendering.screenshot.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: frontend-tests
  • GitHub Check: backend-tests
  • GitHub Check: build-smoketest-push

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

  • Public repositories are always opted into early access features.
  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
frontend/javascripts/test/puppeteer/dtype_datasets_rendering.screenshot.ts (2)

240-247: Flatten async flow in page.evaluate (avoid mixing await with .then).

Good call to await tracing.save() before proceeding. Minor clean-up: prefer a straight await chain for readability.

-            await page.evaluate(async (actions) => {
-              await window.webknossos.apiReady().then(async () => {
-                for (const action of actions) {
-                  window.webknossos.DEV.store.dispatch(action);
-                }
-                await window.webknossos.DEV.api.tracing.save();
-              });
-            }, actions);
+            await page.evaluate(async (actions) => {
+              await window.webknossos.apiReady();
+              for (const action of actions) {
+                window.webknossos.DEV.store.dispatch(action);
+              }
+              await window.webknossos.DEV.api.tracing.save();
+            }, actions);

286-291: Remove stray semicolon and optionally await API readiness
Delete the extra semicolon after save() on line 290. For consistency with the earlier page.evaluate call (around line 241), you can also prepend await window.webknossos.apiReady(); at the top of this callback.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 634dc16 and e981dbe.

⛔ Files ignored due to path filters (10)
  • frontend/javascripts/test/screenshots/2017-05-31_mSEM_aniso-test.png is excluded by !**/*.png
  • frontend/javascripts/test/screenshots/2017-05-31_mSEM_scMS109_bk_100um_v01-aniso.png is excluded by !**/*.png
  • frontend/javascripts/test/screenshots/ROI2017_wkw.png is excluded by !**/*.png
  • frontend/javascripts/test/screenshots/ROI2017_wkw_with_mapping_astrocyte.png is excluded by !**/*.png
  • frontend/javascripts/test/screenshots/annotation_ROI2017_wkw_segmentation.png is excluded by !**/*.png
  • frontend/javascripts/test/screenshots/annotation_ROI2017_wkw_without_fallback.png is excluded by !**/*.png
  • frontend/javascripts/test/screenshots/connectome_file_test_dataset.png is excluded by !**/*.png
  • frontend/javascripts/test/screenshots/kiwi.png is excluded by !**/*.png
  • frontend/javascripts/test/screenshots/test-agglomerate-file_with_mapping_link.png is excluded by !**/*.png
  • frontend/javascripts/test/screenshots/test-agglomerate-file_with_meshes_link.png is excluded by !**/*.png
📒 Files selected for processing (1)
  • frontend/javascripts/test/puppeteer/dtype_datasets_rendering.screenshot.ts (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build-smoketest-push
  • GitHub Check: backend-tests

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, LGTM, just one small suggestion.

await page.evaluate((actions) => {
return window.webknossos.apiReady().then(() => {
await page.evaluate(async (actions) => {
await window.webknossos.apiReady().then(async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The then callback actually gets api as an argument, so you could use that instead of window.webknossos.DEV.api.

@philippotto philippotto enabled auto-merge (squash) September 30, 2025 13:48
@philippotto philippotto merged commit 0e64ad9 into master Sep 30, 2025
5 checks passed
@philippotto philippotto deleted the fix-nightly-25-09-26 branch September 30, 2025 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants